Skip to content

[WC-3464] Rich text Tiptapeditor#2270

Open
gjulivan wants to merge 3 commits into
mainfrom
rich-text/tiptapeditor
Open

[WC-3464] Rich text Tiptapeditor#2270
gjulivan wants to merge 3 commits into
mainfrom
rich-text/tiptapeditor

Conversation

@gjulivan

Copy link
Copy Markdown
Collaborator

Pull request type


Description

@gjulivan
gjulivan requested a review from a team as a code owner June 17, 2026 11:49
@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch 2 times, most recently from 584fe21 to 96df6fb Compare July 1, 2026 08:34
@github-actions

This comment has been minimized.

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch from 96df6fb to 57a6695 Compare July 1, 2026 08:53
@github-actions

This comment has been minimized.

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch from 57a6695 to 3ca1f8c Compare July 2, 2026 21:56
@github-actions

This comment has been minimized.

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch 8 times, most recently from 32f038c to e544a1d Compare July 13, 2026 00:08
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch from c4bbd17 to 6397d1b Compare July 13, 2026 20:17
@github-actions

This comment has been minimized.

@yordan-st

Copy link
Copy Markdown
Contributor

Would have a look at the high and medium flagged CC review comments

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch 4 times, most recently from f7d16ca to 2755b5d Compare July 21, 2026 11:15
@github-actions

This comment has been minimized.

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch 2 times, most recently from 8235fae to cb575ad Compare July 22, 2026 10:53
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch 4 times, most recently from c2054bf to 5d26d7e Compare July 24, 2026 07:22
@github-actions

This comment has been minimized.

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch 4 times, most recently from 10f31ef to 5925c7a Compare July 24, 2026 10:56
@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch from 5925c7a to bd5d88b Compare July 24, 2026 10:58
@github-actions

This comment has been minimized.

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch from bd5d88b to 6d6c750 Compare July 24, 2026 11:24
@github-actions

This comment has been minimized.

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch from 6d6c750 to cd882a6 Compare July 24, 2026 11:49
@github-actions

This comment has been minimized.

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch from cd882a6 to 9bf1d12 Compare July 24, 2026 12:28
@github-actions

This comment has been minimized.

@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch from 9bf1d12 to 469eaa6 Compare July 24, 2026 15:40
@gjulivan
gjulivan force-pushed the rich-text/tiptapeditor branch from 469eaa6 to aa56ad2 Compare July 24, 2026 17:10
@github-actions

Copy link
Copy Markdown
Contributor

AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed


What was reviewed

File Change
packages/pluggableWidgets/rich-text-web/e2e/RichText.spec.js New tests added: class mode editor, class-based HTML assertions, clear-content test
packages/pluggableWidgets/rich-text-web/CHANGELOG.md Unreleased section with new features, fixes, and a breaking change
packages/pluggableWidgets/rich-text-web/package.json New Tiptap v3 dependencies added, several old deps removed
packages/pluggableWidgets/rich-text-web/rollup.config.mjs Added json() and SVG alias plugins
packages/pluggableWidgets/rich-text-web/src/RichText.editorConfig.ts Added helpButton to toolbarGroupKeys
packages/pluggableWidgets/rich-text-web/openspec/ Large number of OpenSpec change artifacts added (docs only)
packages/pluggableWidgets/rich-text-web/e2e/RichText.spec.js-snapshots/ Two snapshot baselines updated

Skipped (out of scope): dist/, pnpm-lock.yaml, openspec markdown docs


Findings

🔶 Medium — E2E tests failing in CI

File: packages/pluggableWidgets/rich-text-web/e2e/RichText.spec.js
Problem: The prior CI run (30106298121) failed on "Run automated end-to-end tests (0)" with screenshot diffs uploaded as artifact test-screenshot-results-0. The tests must be green before merge.
Fix: Re-run the E2E tests after committing the missing snapshot baselines (see finding below) and verify all screenshot comparisons pass.


🔶 Medium — Missing snapshot baselines for new tests

File: packages/pluggableWidgets/rich-text-web/e2e/RichText.spec.js lines 125, 170
Problem: Two new tests call toHaveScreenshot() with names that have no committed baseline PNG in the snapshots directory:

  • classModeEditor.png (referenced on line 125)
  • classModeViewCodeDialog.png (referenced on line 170)

Without a committed baseline Playwright will fail on the first run (or silently write a new baseline, making CI non-deterministic on subsequent runs).
Fix: Run the E2E suite with --update-snapshots to generate these two baselines, then commit classModeEditor-chromium-linux.png and classModeViewCodeDialog-chromium-linux.png to the snapshots directory.


🔶 Medium — page.waitForTimeout() (hardcoded sleep) in test

File: packages/pluggableWidgets/rich-text-web/e2e/RichText.spec.js line 212
Problem: await page.waitForTimeout(500) is a hardcoded sleep. Per the E2E test guidelines this is prohibited — it makes tests flaky on slow machines and wastes time on fast ones.
Fix: Replace with a web-first locator assertion that waits for the observable side-effect. For the empty-content test this is likely waiting for the blur/save cycle; assert the editor's content state directly:

// Instead of await page.waitForTimeout(500):
await expect(page.locator(".mx-name-richText1 .tiptap")).toHaveText("");
// or wait for the bound attribute to reflect the empty value if accessible in the DOM

⚠️ Low — PR description not filled in

File: PR body
Note: The PR template sections ("Pull request type" and "Description") are all still commented out. None of the type checkboxes are uncommented and there is no written description. This makes it hard for reviewers and future git archaeology to understand what this PR does. Please uncomment the relevant type(s) and add a summary of the changes.


⚠️ Low — Breaking change not called out prominently in CHANGELOG

File: packages/pluggableWidgets/rich-text-web/CHANGELOG.md
Note: The entry "Empty content now correctly saves as "" ... This is a breaking change for forms that were relying on the incorrect behavior" is placed under ### Fixed with only an inline note. Per Keep a Changelog conventions, breaking changes belong in a dedicated ### Changed or ### Breaking Changes section (or at a minimum get a **Breaking:** prefix) so consumers scanning the changelog don't miss the behavior change. Consider promoting this to its own callout.


Positives

  • All new E2E tests use @mendix/run-e2e/fixtures and waitForMendixApp — no raw Playwright imports or manual logout.
  • The class-mode assertion test (checks that class mode editor output uses CSS classes instead of inline styles) uses semantic HTML assertions rather than screenshot-only verification — much more robust for logic coverage.
  • Snapshot files for updated tests (inlineBasicMode, toolbarBasicMode) are committed alongside the spec changes.
  • editorConfig.ts correctly adds helpButton to toolbarGroupKeys to keep the hide-properties logic aligned with the new XML property.
  • CHANGELOG entry is detailed, user-facing, and covers the security CVE fix, new features, and the breaking behavior change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants